Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed error when trying to drop ammo for weapons without AmmoEnt #1671

Merged
merged 5 commits into from
Nov 27, 2024

Conversation

MrXonte
Copy link
Contributor

@MrXonte MrXonte commented Oct 22, 2024

When trying to drop ammo on a weapon with no AmmoEnt (aka AmmoEnt = ""), an error appears in the console from trying to create a non-existent entity.

@MrXonte
Copy link
Contributor Author

MrXonte commented Oct 22, 2024

i just noticed that AmmoEnt isnt default in the base anyway, but it seems quite a few addons do add it with = "". Guess the addons should update instead of TTT2 then

@MrXonte MrXonte closed this Oct 22, 2024
@TimGoll
Copy link
Member

TimGoll commented Oct 22, 2024

Uhm, am I missing something or is your fix an actual valid fix? .AmmoEnt is indeed used in TTT2 (and in vanilla TTT as well):

SWEP.AmmoEnt = "item_ammo_smg1_ttt"

@MrXonte
Copy link
Contributor Author

MrXonte commented Oct 22, 2024

Yea as per my comment i noticed that the issue is within addons, not TTT/TTT2 itself. A few addons use AmmoEnt = "" instead of just not setting AmmoEnt in SWEPS. If its not set the default code works perfectly fine, (due to the nil check) but if an Addon sets it to "", then it causes Errors. Should be the addon creators issue, that's why I closed the pull request, my bad

@TimGoll
Copy link
Member

TimGoll commented Oct 22, 2024

Yea as per my comment i noticed that the issue is within addons, not TTT/TTT2 itself. A few addons use AmmoEnt = "" instead of just not setting AmmoEnt in SWEPS. If its not set the default code works perfectly fine, (due to the nil check) but if an Addon sets it to "", then it causes Errors. Should be the addon creators issue, that's why I closed the pull request, my bad

Understood. But adding a failsafe check to TTT2 is perfectly reasonable in my opinion, even though the devs should set it to nil instead of "". Keep in mind that TTT2 is still actively maintained, while other addons are not.

Imho your fix sounds like a good addition. @Histalek should voice his opinion as well I think

@Histalek
Copy link
Member

Could you send us an example of such an error?

As far as i can see we already check for invalid entity classes in this case, so i'm confused on how this could possibly lead to an error

@MrXonte
Copy link
Contributor Author

MrXonte commented Oct 23, 2024

image
Here i tried to drop ammo with a SWEP that has SWEP.AmmoEnt = "". Its not a "loud" error that gets reported in the problems section, but its one of the red "silent" errors.
This error also happens when using random weapon spawns with matching ammo

@Histalek
Copy link
Member

Histalek commented Nov 6, 2024

Ok i think i understand now.

The Attempted to create unknown entity type <the entity type in question>! is caused by the call to ents.Create [1] here [2].
We already check if the returned result is valid and prevent this becoming a lua error, but the console message is directly caused by the function call.
Sadly i don't know of a way to check beforehand if an entity class exists and therefore we cannot guard against this.

So yeah this is a problem best solved on the individual swep addon side.

However if "" would be an extremely widely used ammoEnt i would be fine with the PR to prevent this specific case

[1] https://wiki.facepunch.com/gmod/ents.Create

[2]

local box = ents.Create(wep.AmmoEnt)

@TimGoll
Copy link
Member

TimGoll commented Nov 7, 2024

I agree with @Histalek and this is also how I understood it. Although I did not spend as much time to understand the real reason behind all this. I'll reopen this PR. If you, @MrXonte are against it, feel free to say so. Thanks for raising this issue!

@TimGoll TimGoll reopened this Nov 7, 2024
@Histalek Histalek self-assigned this Nov 15, 2024
@Histalek
Copy link
Member

I've merged the check into our existing check and wrote a reasoning comment so we don't forget

Thanks again @MrXonte for finding and fixing this!

@Histalek Histalek added this to the v0.14.1b milestone Nov 15, 2024
@Histalek Histalek requested a review from saibotk November 27, 2024 15:01
@Histalek Histalek enabled auto-merge (squash) November 27, 2024 15:27
@Histalek Histalek merged commit 2493ff1 into TTT-2:master Nov 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants